Skip to content

HDDS-6182 Basic framework for Client and OM versioning #3031

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
kerneltime wants to merge 1 commit into from
Closed

HDDS-6182 Basic framework for Client and OM versioning #3031

kerneltime wants to merge 1 commit into from

Conversation

@kerneltime
Copy link
Contributor

@kerneltime kerneltime commented Feb 1, 2022

What changes were proposed in this pull request?

This change introduces the basic framework for

  1. OM to be able to change the behavior of a request processing based on the client version.
  2. Client to be able to check the version of OM.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-6182

How was this patch tested?

Basic unit tests

rakeshadr
rakeshadr reviewed Feb 2, 2022
View reviewed changes
Copy link
Contributor

@rakeshadr rakeshadr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kerneltime for the contribution. Added few comments, please go through it.

@kerneltime kerneltime force-pushed the HDDS-6182 branch 2 times, most recently from 555c281 to bb3378b Compare February 3, 2022 00:40
adoroszlai
adoroszlai reviewed Feb 8, 2022
View reviewed changes
Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kerneltime for working on this.

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OMProtocolVersion.java
Comment on lines +29 to +31
public static final String OM_SUPPORTS_S3AUTH_VIA_PROTO = "2.0.0";
public static final String OM_SUPPORTS_EC = "2.1.0";
public static final String OM_SUPPORTS_FSO = "2.1.0";
Copy link
Contributor

@adoroszlai adoroszlai Feb 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the benefit of having a 3-digit version number? When would we increment each of the digits?

Copy link
Contributor Author

@kerneltime kerneltime Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is follows semantic versioning... https://semver.org/
This will allow us to introduce breaking changes and bump up the highest digit for them.

Copy link
Contributor

@adoroszlai adoroszlai Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have semantic versioning at the project level. I don't think we can introduce breaking changes without incrementing the project-level major version number.

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/ClientVersions.java
*/
public static boolean isClientCompatible(int desiredClientVersion,
int actualClientVersion) {
return desiredClientVersion >= actualClientVersion;
Copy link
Contributor

@adoroszlai adoroszlai Feb 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "client version is equal to or newer" is opposite of desiredClientVersion >= actualClientVersion.

Suggested change
return desiredClientVersion >= actualClientVersion;
return desiredClientVersion <= actualClientVersion;

Copy link
Contributor Author

@kerneltime kerneltime Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clients should be able to talk to older servers, a client can choose to mandate a minimum version for the server but a server can mandate the minimum version of client for a given capability.

Copy link
Contributor

@adoroszlai adoroszlai Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a server can mandate the minimum version of client for a given capability.

I think I understand that. But the method does exactly the opposite, it mandates a maximum version.

Copy link
Contributor

@adoroszlai adoroszlai Feb 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To confirm this problem, try:

  1. Increase CURRENT_VERSION to 4 (i.e. simulate the situation where we add a new feature and increment current version)
  2. Set log level of OMManagerClientVersionValidations to DEBUG
om_1        | 2022-02-10 19:03:36,205 [IPC Server handler 0 on default port 9862] DEBUG protocolPB.OMManagerClientVersionValidations: Request rejected as client version is not EC Compatible: cmdType: InfoBucket
om_1        | traceID: ""
om_1        | clientId: "client-585C0CCB3EA6"
om_1        | version: 4
om_1        | infoBucketRequest {
om_1        |   volumeName: "vol1"
om_1        |   bucketName: "bucket1"
om_1        | }
om_1        |
...
om_1        | 2022-02-10 19:04:11,472 [IPC Server handler 5 on default port 9862] DEBUG protocolPB.OMManagerClientVersionValidations: Request rejected as client version is not FSO Compatible: cmdType: LookupKey
om_1        | traceID: ""
om_1        | clientId: "client-55BD1FDBB8B5"
om_1        | version: 4
om_1        | lookupKeyRequest {
om_1        |   keyArgs {
om_1        |     volumeName: "vol1"
om_1        |     bucketName: "bucket1"
om_1        |     keyName: "etc/passwd"
om_1        |     dataSize: 0
om_1        |     sortDatanodes: false
om_1        |     latestVersionLocation: true
om_1        |     headOp: false
om_1        |   }
om_1        | }

Notice that client is version: 4, but is "rejected".

hadoop-hdds/common/src/test/java/org/apache/hadoop/OMProtocolVersionTest.java
Comment on lines +83 to +84
void isOMCompatible() {
}
Copy link
Contributor

@adoroszlai adoroszlai Feb 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be incomplete.

hadoop-hdds/common/src/test/java/org/apache/hadoop/OMProtocolVersionTest.java

import static org.apache.hadoop.ozone.OMProtocolVersion.OM_LATEST_VERSION;

class OMProtocolVersionTest {
Copy link
Contributor

@adoroszlai adoroszlai Feb 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ozone build expects tests to be named Test*, not *Test. So this class and RpcClientTest are not run at all in CI.

Copy link
Contributor Author

@kerneltime kerneltime Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reminder.

@snyk-bot snyk-bot mentioned this pull request Feb 14, 2022
Open
@errose28 errose28 mentioned this pull request Feb 17, 2022
Merged
@adoroszlai adoroszlai closed this Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adoroszlai adoroszlai adoroszlai left review comments

@rakeshadr rakeshadr rakeshadr left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kerneltime @adoroszlai @rakeshadr